-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make GetVersion more deterministic #1807
Make GetVersion more deterministic #1807
Conversation
temporal-sdk/src/main/java/io/temporal/internal/common/SdkFlag.java
Outdated
Show resolved
Hide resolved
temporal-sdk/src/main/java/io/temporal/internal/common/SdkFlag.java
Outdated
Show resolved
Hide resolved
temporal-sdk/src/main/java/io/temporal/internal/common/SdkFlag.java
Outdated
Show resolved
Hide resolved
public static SdkFlag GetValue(int _id) { | ||
SdkFlag[] As = SdkFlag.values(); | ||
for (int i = 0; i < As.length; i++) { | ||
if (As[i].Compare(_id)) return As[i]; | ||
} | ||
return SdkFlag.UNKNOWN; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static SdkFlag GetValue(int _id) { | |
SdkFlag[] As = SdkFlag.values(); | |
for (int i = 0; i < As.length; i++) { | |
if (As[i].Compare(_id)) return As[i]; | |
} | |
return SdkFlag.UNKNOWN; | |
} | |
public static Optional<SdkFlag> fromOrdinal(int ordinal) { | |
if ordinal < 0 || ordinal >= SdkFlag.values().length - 1 { | |
return Optional.empty(); | |
} | |
return Optional.of(SdkFlag.values()[ordinal]); | |
} |
Might as well just resolve to always match the index and keep it optional instead of adding an unusable value to the class.
temporal-sdk/src/main/java/io/temporal/internal/common/SdkFlags.java
Outdated
Show resolved
Hide resolved
* Changes behaviour of GetVersion to not yield if no previous call existed in history. | ||
*/ | ||
SKIP_YIELD_ON_DEFAULT_VERSION(1), | ||
UNKNOWN(Integer.MAX_VALUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have an unknown flag? Just make the return from GetValue
(sic) as Optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNKNOWN
is not the same as an empty optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm saying is it shouldn't be the same. You should only have enumerates for real flags IMO. There's no reason to have all but one enumerate be a real flag IMO. Make it clear to the caller that the flag is unknown, not just that it happens to have another name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think returning UNKNOWN
is clear to the caller that the flags meaning is not know. This is how we handle it in the other SDKs that implement flags as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eek. I hope we at least don't in TS, Python, and .NET. All those languages have a good way of representing an absent enum without having to work around that some values of the enum are representable in history and some aren't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it was an absent enum I would use optional, but it really is the lack of knowledge about how to handle a value. I would rather leave it as is in Java since I think it is clearer and more consistent with other SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree, but y'all's design choice here. At least it's internal.
temporal-sdk/src/main/java/io/temporal/internal/statemachines/WorkflowStateMachines.java
Outdated
Show resolved
Hide resolved
temporal-sdk/src/main/java/io/temporal/internal/sync/SyncWorkflowContext.java
Show resolved
Hide resolved
...oral-sdk/src/test/java/io/temporal/workflow/versionTests/GetVersionInSignalOnReplayTest.java
Outdated
Show resolved
Hide resolved
4530654
to
354c471
Compare
Fix a possible source of non determinism in
GetVersion
calls. SinceGetVersion
can block on a future adding a call to it in a workflow can lead to non determinism so we need to avoid doing so in a backwards compatible way.I tried to refactor the Version state machine to avoid ever blocking, but found the work too complicated for the benefit since due to backwards compatibility we can never remove the old state machine.
closes #1430